Skip to content

Conversation

@cstranex
Copy link

@cstranex cstranex commented Oct 5, 2024

Currently Sentry will emit an unhandled exception for StopIteration. However,
when running certain combinations of libraries with gunicorn and sync workers
(eg: python-socketio) the StopIteration exception is actually expected. To
workaround this issue, don't capture this exception when using this middleware
with gunicorn and just reraise it.

See discussion about StopIteration here: miguelgrinberg/python-engineio#335 and miguelgrinberg/flask-sock#64

Currently Sentry will emit an unhandled exception for StopIteration. However,
when running certain combinations of libraries with gunicorn and sync workers
(eg: python-socketio) the StopIteration exception is actually expected. To
workaround this issue, don't capture this exception when using this middleware
with gunicorn and just reraise it.
@szokeasaurusrex
Copy link
Member

Hey @cstranex thanks for the contribution. I am a bit hesitant though to merge this change as is; it seems like gunicorn only is expected to raise the StopIteration exception in certain situations, which may not apply to all users. The change that you propose would always cause StopIteration to be ignored when using gunicorn, although we would only want to ignore the exception when we know gunicorn will handle it.

Is there any way (perhaps by inspecting the exception message) that we could ensure that we only suppress StopIteration when gunicorn will handle it?

@codecov
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.22%. Comparing base (00f8140) to head (c289264).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3616      +/-   ##
==========================================
- Coverage   84.27%   84.22%   -0.06%     
==========================================
  Files         133      133              
  Lines       14024    14028       +4     
  Branches     2955     2958       +3     
==========================================
- Hits        11819    11815       -4     
- Misses       1465     1478      +13     
+ Partials      740      735       -5     
Files with missing lines Coverage Δ
sentry_sdk/integrations/wsgi.py 95.45% <100.00%> (+0.14%) ⬆️

... and 4 files with indirect coverage changes

@cstranex
Copy link
Author

cstranex commented Oct 7, 2024

@szokeasaurusrex Hmm, for the two libraries linked the StopIteration exception has a blank message so it will be difficult to know exactly where it comes from without asking the library maintainers to make some changes for us to be able to identify if it should be caught. I had a similar idea using the stack trace but I think that's maybe bit too brittle.

At least for the case I'm looking at we are opening websocket connections and the StopIteration gets issued to request gunicorn to close the socket when a client disconnects. If we are only dealing with this case then we can inspect the WSGI environ for an Connection: Upgrade header which should only present for websockets. If it is present then we can ignore the StopIteration, otherwise we capture it?

@cstranex
Copy link
Author

cstranex commented Oct 7, 2024

I suppose another way to handle it could be a way to specify uncaught exceptions that should be ignored either by a list or by a callback

@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Oct 7, 2024

I suppose another way to handle it could be a way to specify uncaught exceptions that should be ignored either by a list or by a callback

@cstranex If all you want to do is have these StopIteration exceptions be suppressed for your setup, you can simply use a custom before_send callback to filter out the StopIteration exceptions you wish to ignore.

If this is not a very widespread problem, and if this solution works for you, I think just having users who wish to ignore these exceptions configure a custom before_send would be the best solution. The logic to infer when we can safely ignore a StopIteration exception will likely always be a bit brittle, so having users configure the logic to their needs is maybe the most robust and least error prone option here.

@cstranex
Copy link
Author

cstranex commented Oct 7, 2024

Fair enough, I was thinking it is probably better to ignore it universally since it is somewhat expected for gunicorn to handle it, but I agree it is probably not common enough that it could just be filtered out on a per usage basis

@cstranex cstranex closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants